-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Slim Down #21
Slim Down #21
Conversation
* Removes request in favor of axios. * Removes entire lodash package in favor of individual lodash packages.
* jwks-rsa already has axios as a dependent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing broken, only suggestions to make it even smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good from an infosec perspective. I'm able to confirm that this PR doesn't change any functionality with how it previously worked. Nice job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small comment otherwise looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give this the ol' :stamp: on behalf of 360-web. As we start to roll this out, are there particular things we should have QA look for?
Not really. Just regressions, i think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always down for adding Lambda support. The only suggestion/thought I'd have is that you can replace much of Ramda with tinyfunk. You'd get to keep the functional, but seriously cut down on file-size. Totally just a thought, though, since I see you've solved things in more imperative ways in some places.
@mgreystone, one more question: If the concern is for Lambda package size limits, have you considered using Lambda Layers? |
I have not. I'm not sure i follow how i'd use layers to solve this problem? Especially in a lambda@edge? This does shave just shy of 2 MB the size of my lambda package, but i'm honestly not confident it's going to be enough... |
The idea with layers is that you can put all of your deps in a layer, and your actual lambda code in the function itself. Each layer can be up to the max zip-file size supported by Lambda, and you can have up to 5 layers per function. So it increases the size limit of your uploads. I've built Lambda's before where all the node modules were in a layer, and then the lambda function was just a single file. It's not a one-size-fits-all solution, and I'm not sure if our Jenkins flows supports it, but layers can help with upload size. |
I'd like to use authentic in a lambda@edge. Unfortunately, there are strict size limits, & authentic would need to be slimmed down significantly. This is my proposal:
jwks-rsa
, which dropsrequest
in favor ofaxios
& dropslodash
gimme
in favor ofaxios
, sincejwks-rsa
is using it alreadyfunky
ramda
Misc